Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consolidate VS detection logic #49593

Merged
merged 2 commits into from
Mar 14, 2021
Merged

Consolidate VS detection logic #49593

merged 2 commits into from
Mar 14, 2021

Conversation

AntonLapounov
Copy link
Member

@AntonLapounov AntonLapounov commented Mar 14, 2021 •
edited
Loading

Consolidate and improve Visual Studio installation detection logic. Fixes #1251.

At present we use four almost identical code snippets in different script files for detection Visual Studio installation. They all run vswhere.exe, check Visual Studio version, launch Developer Command Prompt, and optionally set VC++ build environment, check DIA SDK's presence, and set CMakePath environment variable. The goal of this PR is to consolidate that logic in a single script to improve maintainability.

The current VS detection logic has the following flaw. If you have several VS installations on the machine, it may choose a VS installation with no VC Tools component present, which will fail any native build. Since DIA SDK is always installed with the VC Tools component, checking for DIA SDK's presence in our scripts is more or less equivalent to checking for the VC Tools component's presence. This PR fixes the described flaw by choosing only among VS installations with the VC Tools component present (still choosing the latest VS if there are many).

One consequence of this PR will be failing the build earlier if there is no VS installation with the VC Tools component present. If we think purely managed builds with no VC Tools installed on the computer is a scenario worth supporting, I can relax the VSWhere's filter to require the VC Tools component only when the native architecture is passed to init-vs-env.cmd.

Below I summarized the relevant steps different scripts would execute before and after my changes.

Before After
eng\native\init-vs-env.cmd
This script did not exist
 
 
 		 • Runs VSWhere and calls VsDevCmd.bat -no_logo
• VC Tools component check guarantees DIA SDK's presence
Only if architecture is passed as an argument:
• Calls vcvarsall.bat %__VCBuildArch%
• Sets CMakePath
src\coreclr\build-runtime.cmd
• Runs VSWhere and calls VsDevCmd.bat (via setup_vs_tools.cmd)
Only if native build is needed:
• Calls vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets CMakePath		 Same list but calls init-vs-env.cmd instead of setup_vs_tools.cmd
in the first step (checking for VC Tools unconditionally instead of
the conditional DIA SDK check)
src\coreclr\crossgen-corelib.cmd
• Runs VSWhere and calls VsDevCmd.bat (via setup_vs_tools.cmd)
• Calls vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence		 Calls init-vs-env.cmd %__BuildArch%, which does all the steps on the left (checking for VC Tools instead of DIA SDK)
src\coreclr\crossgen-corelib.proj
• Runs VSWhere and calls VsDevCmd.bat (via setup_vs_tools.cmd)		 Same but calls init-vs-env.cmd instead of setup_vs_tools.cmd
src\coreclr\setup_vs_tools.cmd
• Runs VSWhere and calls VsDevCmd.bat		 This script is replaced by init-vs-env.cmd
src\libraries\Native\build-native.cmd
• Runs VSWhere and calls VsDevCmd.bat -no_logo
• Calls vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets CMakePath		 Calls init-vs-env.cmd %__BuildArch%, which does all the steps on the left (checking for VC Tools instead of DIA SDK)
src\mono\mono.proj
• Calls init-compiler-and-cmake.cmd $(Platform) (see the next line)		 Calls init-vs-env.cmd $(Platform)
eng\native\init-compiler-and-cmake.cmd
• Runs VSWhere and calls VsDevCmd.bat -no_logo
• Calls vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets CMakePath		 This script is replaced by init-vs-env.cmd
src\native\corehost\build.cmd
• Runs VSWhere and calls VsDevCmd.bat
• Calls vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets CMakePath		 Calls init-vs-env.cmd %__BuildArch%, which does all the steps on the left (checking for VC Tools instead of DIA SDK)
src\tests\build.cmd
• Runs VSWhere and calls VsDevCmd.bat (via setup_vs_tools.cmd)
Only if native build is needed:
• Calls vcvarsall.bat %__VCBuildArch%
• Checks DIA SDK's presence
• Sets CMakePath		 Same list but calls init-vs-env.cmd instead of setup_vs_tools.cmd
in the first step (checking for VC Tools unconditionally instead of
the conditional DIA SDK check)

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Mar 14, 2021

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Consolidate and improve Visual Studio installation detection logic. Contributes to #1251.

At present we use four almost identical code snippets in different script files for detection Visual Studio installation. They all run vswhere.exe, check Visual Studio version, launch Developer Command Prompt, and optionally set VC++ build environment, check DIA SDK existence, and set CMakePath environment variable. The goal of this PR is to consolidate that logic in a single script.

Author: AntonLapounov
Assignees: -
Labels:

area-Infrastructure
Milestone: -

@AntonLapounov
Copy link
Member Author

The Mono failure is #49569.

@AntonLapounov
Copy link
Member Author

Cc @trylek regarding the plan he suggested in #38919 (comment).

jkotas
jkotas reviewed Mar 14, 2021
View reviewed changes
eng/native/init-vs-env.cmd Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented Mar 14, 2021

Contributes to #1251.

Is there anything else to do to fix #1251? I think that this is complete fix.

@AntonLapounov
Copy link
Member Author

Yes, I hope we can close #1251 with this fix. I was going to check that with @danmoseley.

@danmoseley
Copy link
Member

Sounds like a fix to me. Thanks for the cleanup!

jkotas
jkotas approved these changes Mar 14, 2021
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ViktorHofer
Copy link
Member

One consequence of this PR will be failing the build earlier if there is no VS installation with the VC Tools component present. If we think purely managed builds with no VC Tools installed on the computer is a scenario worth supporting, I can relax the VSWhere's filter to require the VC Tools component only when the native architecture is passed to init-vs-env.cmd.

Just to double check, I don't see a reference to the consolidated script from the base build.ps1/build.sh scripts which makes me believe that managed builds don't fail early, right? I would expect to be able to i.e. build the libs.ref subset without a prereq check.

@ViktorHofer
Copy link
Member

OK just saw that your comment refers to the native check in the build-runtime.cmd and in the src/tests/build.cmd script. I think it's fine to unconditionally check for the VS installation.

trylek
trylek approved these changes Mar 14, 2021
View reviewed changes
Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome cleanup, thank you!

@jkotas jkotas merged commit c156959 into dotnet:main Mar 14, 2021
@AntonLapounov AntonLapounov deleted the ConsolidateVSDetection branch March 14, 2021 22:08
@akoeplinger
Copy link
Member

Thank you for this cleanup!

@runfoapp runfoapp bot mentioned this pull request Mar 15, 2021
Open
@hoyosjs hoyosjs mentioned this pull request Mar 16, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trylek trylek trylek approved these changes

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@jkotas jkotas jkotas approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees
No one assigned
Labels
area-Infrastructure
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Centralize and improve Windows prereq checks
7 participants
@AntonLapounov @jkotas @danmoseley @ViktorHofer @akoeplinger @trylek @karelz